Skip to content

feat(cli): parallel catalog validation and fleet rollup - #123

Merged
chrisknvidia merged 24 commits into
NVIDIA:mainfrom
mimran-khan:feat/parallel-catalog-workers
Sep 15, 2026
Merged

feat(cli): parallel catalog validation and fleet rollup#123
chrisknvidia merged 24 commits into
NVIDIA:mainfrom
mimran-khan:feat/parallel-catalog-workers

Conversation

@mimran-khan

@mimran-khan mimran-khan commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #122
Related: #120, #121

This branch includes two catalog features that stack cleanly:

  1. catalog-summary.json ([FEA]: Machine-readable catalog fleet rollup report #120 / feat(cli): write catalog-summary.json after catalog validate #121): fleet rollup after catalog validate
  2. --workers N ([FEA]: Parallel catalog validation with --workers #122): parallel child-process validation for catalogs

--workers defaults to 1 (serial, same per-skill pipeline view as today). Above 1, skills validate in isolated processes with per-skill output dirs under -o. The parent Click context (or sys.argv when launched from the real CLI) is used to rebuild each child validate invocation, so pytest and production entry points both work.

Parallel mode skips the per-skill Rich pipeline view and prints a single fleet scoreboard at the end. Exit code stays nonzero if any skill failed.

If #121 lands first, I can rebase this to workers-only; happy to split either way.

Test plan

  • pytest tests/test_commands.py -k catalog
  • Serial catalog tests still pass with --workers 1 (default)
  • --workers 2 integration test on two fixture skills

@mimran-khan
feat(cli): write catalog-summary.json after catalog validate
5b87f8c 
Emit a machine-readable fleet rollup at the reports root with per-skill
status, optional severity totals from child JSON reports, and report paths.
Create the output directory when needed so summary writes survive early
skill failures.

Fixes NVIDIA#120

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
feat(cli): add parallel catalog validation with --workers
416112b 
Catalog validate accepts --workers N to run skills in isolated child
processes. Values above 1 skip the per-skill pipeline view and rebuild
per-skill argv from the parent Click context or sys.argv.

Fixes NVIDIA#122

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
chrisknvidia
chrisknvidia reviewed Aug 29, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
chrisknvidia
chrisknvidia reviewed Aug 29, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
chrisknvidia
chrisknvidia reviewed Aug 29, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
chrisknvidia
chrisknvidia reviewed Aug 29, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
@mimran-khan
fix(cli): stabilize parallel catalog workers and summary writes
8c51e1b 
Rebuild child argv without dropping positional catalog paths, track fresh per-skill
JSON reports instead of stale files, write catalog-summary.json atomically, and
fix --include-skills forwarding for context fallback.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan

mimran-khan commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Worker argv, stale JSON, atomic summary, and --include-skills fixes are pushed. Ready for re-review.

@chrisknvidia

chrisknvidia commented Aug 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@mimran-khan : Re-reviewed at current head a7ea2e7:

  1. Still broken: real CLI worker argv drops option values. _catalog_child_argv_from_sys() now discards every non-flag token, including values such as schema, 95, model names, and paths. A real console run of skillevaluator validate <catalog> --workers 2 --no-llm --no-dedup --checks schema -o <out> exits 1 and reports 0/1 passed. The new CliRunner test passes because it exercises _catalog_child_argv_from_ctx(), not the production sys.argv path.
  2. Still broken: stale JSON is reused when no new JSON is produced. _run_catalog_skill_worker() returns json_report_name=None, but _catalog_skill_entry() treats None as permission to call _latest_skill_json_report(); serial mode also still scans existing output. Reusing -o reproduced current passed: false / reason: validation failed alongside stale overall_passed: true, overall_status: passed, and old severity counts.

The atomic summary writer and --include-skills fixes were independently verified.

rng1995
rng1995 reviewed Aug 31, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
rng1995
rng1995 requested changes Aug 31, 2026
View reviewed changes

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parallel catalog execution still loses real CLI option values, changes explicit CLI-only reporting, and can reuse stale per-skill JSON after a worker produces no report. Focused catalog tests and Ruff/diff checks passed; the single Python 3.12 CI failure is an unrelated flaky NVIDIA bridge socket test. Requesting changes for the reproducible worker-boundary defects in the review threads.

rng1995
rng1995 reviewed Aug 31, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py
@mimran-khan
fix(cli): address rng1995 parallel catalog worker review
9b66899 
Rebuild child argv from Click params, preserve -r cli when selected, track
per-run JSON in serial catalog mode, and stop attaching stale reports when
no new JSON was produced this run.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
merge main into feat/parallel-catalog-workers
51dc43b 
# Conflicts:
#	CHANGELOG.md

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
merge fork
461a379 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
chrisknvidia
chrisknvidia reviewed Sep 3, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
chrisknvidia
chrisknvidia reviewed Sep 3, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
@mimran-khan
merge main into feat/parallel-catalog-workers
1e2470d 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
fix(cli): drop unused passed assignments in catalog loop
2496821 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
fix(cli): honor implicit catalog report defaults and JSON selection
c8ece19 
Do not forward the implicit cli default to catalog workers, and ignore
SARIF sidecars when picking the standard JSON report for fleet rollup.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
merge main into feat/parallel-catalog-workers
5ddd62d 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
Merge remote-tracking branch 'origin/main' into feat/parallel-catalog…
37037cf 
…-workers

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
fix(cli): bind catalog workers to exact validate JSON reports
89b3f1e 
Record the JSON artifact emitted by each validate invocation instead of
rediscovering reports via directory diffs, which missed same-second
overwrites in parallel catalog mode. Also stop pre-creating catalog
summary parents through symlinked paths.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
chore: merge main into feat/parallel-catalog-workers
c9dfc86 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
chore: fix CHANGELOG merge markers after main merge
ae23dfb 
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
rng1995
rng1995 reviewed Sep 12, 2026
View reviewed changes
Comment thread src/skillevaluator/cli.py Outdated
rng1995
rng1995 requested changes Sep 12, 2026
View reviewed changes

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previously reported worker argument, report-format, stale-report, SARIF-selection, and same-second overwrite defects are fixed at the current head, and all 53 focused command tests pass. Required CI is still red: Ruff rejects the new module-global report handoff, and DCO reports fb87d4a and 21cdb16 as unsigned. Please replace the global with invocation-scoped state, fix the sign-offs, and rerun the checks.

rng1995 and others added 3 commits September 12, 2026 00:36
@rng1995
Merge branch 'main' into feat/parallel-catalog-workers
8be8b5e
@mimran-khan
fix(cli): scope validate JSON report handoff with ContextVar
f257265 
Replace module-global report name slot so parallel catalog workers and
Ruff PLW0603 stay clean while preserving exact JSON binding after validate.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
chore: merge main into feat/parallel-catalog-workers
2b8a360 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan

mimran-khan commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Same ContextVar handoff as #121 at 184a95c, merged with main. Parallel workers consume the report name from the child validate context without a process-wide slot. Same-second JSON overwrite fix and exact basename binding are unchanged. Ready for re-review when CI runs on your side.

@rng1995

rng1995 commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

@mimran-khan, this PR is still blocked at 7b85398b. The ContextVar implementation looks good, but the requested cross-invocation isolation regression is still missing. DCO also needs to be repaired for unsigned commits fb87d4a and 21cdb16. Please add the regression, rewrite or re-sign the affected commits, and ping me when it is ready for re-review.

@mimran-khan
test(cli): cover validate JSON report handoff isolation
6119cdd 
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan

mimran-khan commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Head 327ed19: same ContextVar + isolation test as #121 (327ed19). Parallel catalog path unchanged otherwise. Synced with main, requesting re-review.

@mimran-khan

mimran-khan commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@rng1995 when you have a moment, could you re-review from the current head? I do not have permission to re-request reviewers on the upstream repo from my account.

@mimran-khan
test(cli): prove JSON report handoff under overlapping validate runs
8e80909 
Use copy_context and a barrier so two validations record before either
consume; a module-global slot would return the wrong basename.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@rng1995
rng1995 force-pushed the feat/parallel-catalog-workers branch from 0ffb348 to 9b65462 Compare September 14, 2026 16:23
@rng1995

rng1995 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

@mimran-khan - Please address this blocker: #123 (comment) to approve the PR.

@mimran-khan
test(cli): isolate JSON handoff in distinct Context instances
5c4794f 
Overlap record and consume across two threads so a module-global slot
cannot pass; add a regression that shows the old global behavior leaks.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan
mimran-khan force-pushed the feat/parallel-catalog-workers branch from 9b65462 to 5c4794f Compare September 14, 2026 16:38
@mimran-khan

mimran-khan commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the handoff regression feedback on the latest push.

The isolation test now runs two overlapping handoffs in separate contextvars.Context() instances: both lanes call _record_validate_json_report and hit a barrier before either _consume_validate_json_report, so a module-global slot cannot pass. There is also a paired regression that shows the old global-slot behavior loses at least one basename under the same overlap.

DCO: rewrote the branch history to add Signed-off-by on my unsigned commits (including the bind-catalog-workers and CHANGELOG marker commits that were fb87d4a / 21cdb16 on the earlier head).

rng1995
rng1995 approved these changes Sep 14, 2026
View reviewed changes

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 5c4794f. The report handoff is now invocation-scoped with ContextVar, and the regression overlaps record/consume in two distinct Context instances while demonstrating the former module-global leak. Focused tests and Ruff pass locally; DCO and all required CI checks are green. The remaining review concern is addressed.

@rng1995

rng1995 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

@chrisknvidia - Please review and approve the PR.

@chrisknvidia
Merge origin/main into feat/parallel-catalog-workers
763c4b1 
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia merged commit baf43f5 into NVIDIA:main Sep 15, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrisknvidia chrisknvidia chrisknvidia left review comments
@rng1995 rng1995 rng1995 approved these changes
@mosheabr mosheabr Awaiting requested review from mosheabr mosheabr is a code owner
@rmalani-nv rmalani-nv Awaiting requested review from rmalani-nv rmalani-nv is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEA]: Parallel catalog validation with --workers

3 participants

@mimran-khan @chrisknvidia @rng1995